-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[HttpClient] make HttpClient::create() return an AmpHttpClient when amphp/http-client is found but curl is not or too old #18795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HttpClient] make HttpClient::create() return an AmpHttpClient when amphp/http-client is found but curl is not or too old #18795
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the last paragraph of the section HTTP/2 Support
also needs an update IMO
back to ``AmpHttpClient`` if cURL couldn't be found or is too old. Finally, if | ||
``AmpHttpClient`` is not available, it falls back to PHP streams. | ||
If you prefer to select the transport explicitly, use the following classes | ||
to create the client:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code snippet should be updated to show the third altenative too.
http_client.rst
Outdated
|
||
.. note:: | ||
|
||
To use the `AmpHttpClient`_, the `amphp/http-client`_ package must be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this note looks weird. It seems like AmpHttpClient refers to the Symfony class while the link goes to the source repo of the amphp/http-client
package
http_client.rst
Outdated
This component supports both the native PHP streams and cURL to make the HTTP | ||
requests. Although both are interchangeable and provide the same features, | ||
including concurrent requests, HTTP/2 is only supported when using cURL. | ||
This component supports the native PHP streams, `AmpHttpClient`_ and cURL to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this say amphp/http-client
instead as it refers to what can be used as implementation ?
fd617e3
to
d883b01
Compare
Addressed all your comments @stof, thanks! I tried to update the |
http_client.rst
Outdated
Support for HTTP/2 PUSH works out of the box when libcurl >= 7.61 is used with | ||
PHP >= 7.2.17 / 7.3.4: pushed responses are put into a temporary cache and are | ||
used when a subsequent request is triggered for the corresponding URLs. | ||
Support for HTTP/2 PUSH works out of the box when using ``amphp/http-client`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using using Support for HTTP/2 PUSH works out of the box when using a compatible client
, as the compatibility requirements are already documented a few lines above. It would avoid maintaining that info twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way better 👍
…mphp/http-client is found but curl is not or too old
d883b01
to
e159fc1
Compare
Alex, thanks a lot for taking care of contributing the docs for this "old" feature. One less issue pending to complete the 5.1 milestone 💪 |
Fix #13352